Fold always false type checks #99761

MichalStrehovsky · 2024-03-14T12:13:49Z

If we have a program like:

class Never { }

class Program
{
    static void Main()
    {
        Test(null);
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    static void Test(object o)
    {
        if (o is Never)
            Console.WriteLine("Hello!");
        if (o.GetType() == typeof(Never))
            Console.WriteLine("Hello!");
    }
}

We know these checks are never going to be true thanks to the whole program view. Fold these to false.

If we have a program like: ```csharp class Never { } class Program { static void Main() { Test(null); } [MethodImpl(MethodImplOptions.NoInlining)] static void Test(object o) { if (o is Never) Console.WriteLine("Hello!"); if (o.GetType() == typeof(Never)) Console.WriteLine("Hello!"); } } ``` We know these checks are never going to be true thanks to the whole program view. Fold these to `false`.

EgorBo · 2024-03-14T12:40:54Z

Closes #97601 I presume?

src/coreclr/inc/corinfo.h

MichalStrehovsky · 2024-03-14T13:09:00Z

/azp run runtime-nativeaot-outerloop

azure-pipelines · 2024-03-14T13:09:16Z

Azure Pipelines successfully started running 1 pipeline(s).

jkoritzinsky · 2024-03-14T18:44:12Z

Does this optimization kick in for interfaces? If so, can we add a test around IDynamicInterfaceCastable to ensure that we don't fold casts to types that have the DynamicCastableImplementationAttribute or interfaces they implement (as its possible for no types to statically implement these interfaces but casts to them to still succeed)?

MichalStrehovsky · 2024-03-14T20:58:10Z

@EgorBo @jakobbotsch either of your could psychically debug the reason for hitting a Assertion failed '!"Unexpected tree op after call marked as tailcall"' in 'Microsoft.CodeAnalysis.Collections.ImmutableSegmentedList1+Builder[System.__Canon]:System.Collections.IList.Contains(System.Object):ubyte:this' during 'Morph - Global' (IL size 13; hash 0x40e822e6; FullOpts)?

src/coreclr/jit/importer.cpp

jakobbotsch · 2024-03-14T21:01:46Z

@EgorBo @jakobbotsch either of your could psychically debug the reason for hitting a Assertion failed '!"Unexpected tree op after call marked as tailcall"' in 'Microsoft.CodeAnalysis.Collections.ImmutableSegmentedList1+Builder[System.__Canon]:System.Collections.IList.Contains(System.Object):ubyte:this' during 'Morph - Global' (IL size 13; hash 0x40e822e6; FullOpts)?

No good guess from my side without the jitdump, I'm afraid.

MichalStrehovsky · 2024-03-14T21:06:46Z

Does this optimization kick in for interfaces? If so, can we add a test around IDynamicInterfaceCastable to ensure that we don't fold casts to types that have the DynamicCastableImplementationAttribute or interfaces they implement (as its possible for no types to statically implement these interfaces but casts to them to still succeed)?

It does kick in for interfaces. I cannot come up with a targeted test for this in regards to this optimization. I tried writing one, and ended up with exactly the same thing we already test for IDynamicInterfaceCastable.

EgorBo · 2024-03-14T22:14:18Z

@EgorBo @jakobbotsch either of your could psychically debug the reason for hitting a Assertion failed '!"Unexpected tree op after call marked as tailcall"' in 'Microsoft.CodeAnalysis.Collections.ImmutableSegmentedList1+Builder[System.__Canon]:System.Collections.IList.Contains(System.Object):ubyte:this' during 'Morph - Global' (IL size 13; hash 0x40e822e6; FullOpts)?

hm.. I tried to repro it locally on win-x64 using the CI steps and it succesfully completed on your branch, let's see what your new commit will show

MichalStrehovsky · 2024-03-14T22:39:51Z

@EgorBo @jakobbotsch either of your could psychically debug the reason for hitting a Assertion failed '!"Unexpected tree op after call marked as tailcall"' in 'Microsoft.CodeAnalysis.Collections.ImmutableSegmentedList1+Builder[System.__Canon]:System.Collections.IList.Contains(System.Object):ubyte:this' during 'Morph - Global' (IL size 13; hash 0x40e822e6; FullOpts)?

hm.. I tried to repro it locally on win-x64 using the CI steps and it succesfully completed on your branch, let's see what your new commit will show

Yeah, the same happened to me. I hope we don't have nondeterminism :(

MichalStrehovsky · 2024-03-14T22:40:07Z

/azp run runtime-nativeaot-outerloop

azure-pipelines · 2024-03-14T22:40:22Z

Azure Pipelines successfully started running 1 pipeline(s).

EgorBo · 2024-03-14T23:22:25Z

@MichalStrehovsky I suspect you need to change return value in getExactClasses for CoreCLR and R2R, I presume those always return 0 now

MichalStrehovsky · 2024-03-14T23:24:06Z

@MichalStrehovsky I suspect you need to change return value in getExactClasses for CoreCLR and R2R, I presume those always return 0 now

Good point. I'll let the nativeaot outerloop finish and change it tomorrow together with the JitInterface guid.

MichalStrehovsky · 2024-03-15T06:17:11Z

@EgorBo @jakobbotsch either of your could psychically debug the reason for hitting a Assertion failed '!"Unexpected tree op after call marked as tailcall"' in 'Microsoft.CodeAnalysis.Collections.ImmutableSegmentedList1+Builder[System.__Canon]:System.Collections.IList.Contains(System.Object):ubyte:this' during 'Morph - Global' (IL size 13; hash 0x40e822e6; FullOpts)?

hm.. I tried to repro it locally on win-x64 using the CI steps and it succesfully completed on your branch, let's see what your new commit will show

Yeah, the same happened to me. I hope we don't have nondeterminism :(

It's not nondeterministic - the assert is hitting in main. It very likely broke sometime after 136b100 because that's where my branch was when neither me nor Egor could repro the assert locally. I'll grab a jitdump and file is as a bug, it's unrelated to this PR.

MichalStrehovsky · 2024-03-15T06:18:20Z

This is ready for review.

@dotnet/ilc-contrib could someone have a look at the ILC part? @EgorBo could you have a look at the JIT part?

MichalStrehovsky · 2024-03-15T06:38:32Z

/azp run runtime-nativeaot-outerloop

azure-pipelines · 2024-03-15T06:38:41Z

Azure Pipelines successfully started running 1 pipeline(s).

MichalStrehovsky · 2024-03-15T08:37:51Z

Sigh, I think I have a fix for the SPMI failures but want to wait until nativeaot outerloop finishes.

…into fix97601

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ILScanner.cs

jkotas · 2024-03-15T11:41:57Z

src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs

+
+#if !READYTORUN
+            if (result == TypeCompareState.May
+                && (canNeverHaveInstanceOfSubclassOf(type1) || canNeverHaveInstanceOfSubclassOf(type2)))


Can compareTypesForEquality operate on types that are not allocated, but otherwise exist in the system? This change seems to make hidden assumptions about how this API is used by the JIT.

I'll think about this some more. Worst case I'll just pull this part out. I already used up my JitInterface update quota for the month.

It would not be a problem today but it adds a hidden assumption. Removed it. I'll measure how much it impacts things and whether we want to track that as a separate bug.

I know the typeof optimization would help CsWinRT but dont' have numbers.

We can fix that by adding a bool flag that says the query is specifically for System.Type operator==/!= and only execute this logic for that.

It would not be a problem today but it adds a hidden assumption. Removed it. I'll measure how much it impacts things and whether we want to track that as a separate bug.

I removed the wrong part. Fixed it in the latest iteration.

EgorBo

The JIT part specifically LGTM. There is also one use of getExactClasses in the helperexpansion phase, but that one doesn't need any treatment.

Co-authored-by: Jan Kotas <jkotas@microsoft.com>

jkotas · 2024-03-16T17:08:55Z

src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs

@@ -2248,14 +2248,42 @@ private void getFieldInfo(ref CORINFO_RESOLVED_TOKEN pResolvedToken, CORINFO_MET
            //       and STS::AccessCheck::CanAccess.
        }

+        private bool canNeverHaveInstanceOfSubclassOf(TypeDesc type)


Suggested change

private bool canNeverHaveInstanceOfSubclassOf(TypeDesc type)

private bool CanNeverHaveInstanceOfSubclassOf(TypeDesc type)

Nit: We seem to use regular naming convention in this file for methods that are not directly exposed on JIT/EE interface

jkotas · 2024-03-16T17:14:19Z

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ILScanner.cs

+            public override bool CanReferenceConstructedMethodTable(TypeDesc type)
+                => _constructedMethodTables.Contains(type);
+
+            public override bool CanTypeOrCanonicalFormOfTypeBeAllocated(TypeDesc type)


These two methods do very similar things, but they have very different names. Should these two methods have similar names, e.g. CanReferenceConstructedMethodTable + CanReferenceConstructedTypeOrCanonicalFormOfType; CanTypeBeAllocated + CanTypeOrCanonicalFormOfTypeBeAllocated ?

jkotas

LGTM otherwise. Thanks!

MichalStrehovsky · 2024-03-18T10:59:40Z

/azp run runtime-nativeaot-outerloop

azure-pipelines · 2024-03-18T10:59:54Z

Azure Pipelines successfully started running 1 pipeline(s).

MichalStrehovsky added the area-NativeAOT-coreclr label Mar 14, 2024

MichalStrehovsky self-assigned this Mar 14, 2024

github-actions bot mentioned this pull request Mar 14, 2024

99761 MichalStrehovsky/rt-sz#3

Closed

EgorBo reviewed Mar 14, 2024

View reviewed changes

src/coreclr/inc/corinfo.h Show resolved Hide resolved

This was referenced Mar 14, 2024

mono_os_mutex_destroy: pthread_mutex_destroy failed with "Device or resource busy" #99609

Closed

System.DirectoryServices.Protocols.Tests.BerConverterTests.Decode_Bytes_ReturnsExpected failing on Windows x86 #99725

Closed

jakobbotsch reviewed Mar 14, 2024

View reviewed changes

src/coreclr/jit/importer.cpp Outdated Show resolved Hide resolved

MichalStrehovsky added 2 commits March 14, 2024 22:37

FB

a51509a

Merge branch 'main' into fix97601

1da6be4

build-analysis bot mentioned this pull request Mar 15, 2024

Tracking issue for CI build timeouts #76454

Closed

MichalStrehovsky added 2 commits March 15, 2024 06:55

FB

8e36397

Merge branch 'main' into fix97601

22af9c1

MichalStrehovsky marked this pull request as ready for review March 15, 2024 06:17

Update Compilation.csproj

d1e26b3

MichalStrehovsky added 2 commits March 15, 2024 11:41

Fix SPMI

5f01872

Merge branch 'fix97601' of https://github.com/MichalStrehovsky/runtime …

fc2a7cc

…into fix97601

jkotas reviewed Mar 15, 2024

View reviewed changes

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ILScanner.cs Outdated Show resolved Hide resolved

jkotas reviewed Mar 15, 2024

View reviewed changes

EgorBo approved these changes Mar 15, 2024

View reviewed changes

MichalStrehovsky and others added 2 commits March 16, 2024 11:55

FB

83d4e47

Update src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ILScanner.cs

40d7553

Co-authored-by: Jan Kotas <jkotas@microsoft.com>

MichalStrehovsky closed this Mar 16, 2024

MichalStrehovsky reopened this Mar 16, 2024

Update ILScanner.cs

4e24a85

jkotas reviewed Mar 16, 2024

View reviewed changes

jkotas approved these changes Mar 16, 2024

View reviewed changes

MichalStrehovsky added 3 commits March 17, 2024 06:04

FB

f6a3819

I deleted the wrong part...

b8b7a02

Disable the tests that relied on compareTypesForEquality logic

a2891f4

MichalStrehovsky merged commit 21cfd9f into dotnet:main Mar 18, 2024
119 of 121 checks passed

MichalStrehovsky deleted the fix97601 branch March 18, 2024 21:28

github-actions bot locked and limited conversation to collaborators Apr 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fold always false type checks #99761

Fold always false type checks #99761

MichalStrehovsky commented Mar 14, 2024 •

edited

Loading

EgorBo commented Mar 14, 2024

MichalStrehovsky commented Mar 14, 2024

azure-pipelines bot commented Mar 14, 2024

jkoritzinsky commented Mar 14, 2024

MichalStrehovsky commented Mar 14, 2024

jakobbotsch commented Mar 14, 2024

MichalStrehovsky commented Mar 14, 2024

EgorBo commented Mar 14, 2024

MichalStrehovsky commented Mar 14, 2024

MichalStrehovsky commented Mar 14, 2024

azure-pipelines bot commented Mar 14, 2024

EgorBo commented Mar 14, 2024

MichalStrehovsky commented Mar 14, 2024

MichalStrehovsky commented Mar 15, 2024

MichalStrehovsky commented Mar 15, 2024

MichalStrehovsky commented Mar 15, 2024

azure-pipelines bot commented Mar 15, 2024

MichalStrehovsky commented Mar 15, 2024

jkotas Mar 15, 2024

MichalStrehovsky Mar 15, 2024

MichalStrehovsky Mar 16, 2024

jkotas Mar 16, 2024

MichalStrehovsky Mar 18, 2024

EgorBo left a comment

jkotas Mar 16, 2024

jkotas Mar 16, 2024

jkotas left a comment

MichalStrehovsky commented Mar 18, 2024

azure-pipelines bot commented Mar 18, 2024

	private bool canNeverHaveInstanceOfSubclassOf(TypeDesc type)
	private bool CanNeverHaveInstanceOfSubclassOf(TypeDesc type)

Fold always false type checks #99761

Fold always false type checks #99761

Conversation

MichalStrehovsky commented Mar 14, 2024 • edited Loading

EgorBo commented Mar 14, 2024

MichalStrehovsky commented Mar 14, 2024

azure-pipelines bot commented Mar 14, 2024

jkoritzinsky commented Mar 14, 2024

MichalStrehovsky commented Mar 14, 2024

jakobbotsch commented Mar 14, 2024

MichalStrehovsky commented Mar 14, 2024

EgorBo commented Mar 14, 2024

MichalStrehovsky commented Mar 14, 2024

MichalStrehovsky commented Mar 14, 2024

azure-pipelines bot commented Mar 14, 2024

EgorBo commented Mar 14, 2024

MichalStrehovsky commented Mar 14, 2024

MichalStrehovsky commented Mar 15, 2024

MichalStrehovsky commented Mar 15, 2024

MichalStrehovsky commented Mar 15, 2024

azure-pipelines bot commented Mar 15, 2024

MichalStrehovsky commented Mar 15, 2024

jkotas Mar 15, 2024

Choose a reason for hiding this comment

MichalStrehovsky Mar 15, 2024

Choose a reason for hiding this comment

MichalStrehovsky Mar 16, 2024

Choose a reason for hiding this comment

jkotas Mar 16, 2024

Choose a reason for hiding this comment

MichalStrehovsky Mar 18, 2024

Choose a reason for hiding this comment

EgorBo left a comment

Choose a reason for hiding this comment

jkotas Mar 16, 2024

Choose a reason for hiding this comment

jkotas Mar 16, 2024

Choose a reason for hiding this comment

jkotas left a comment

Choose a reason for hiding this comment

MichalStrehovsky commented Mar 18, 2024

azure-pipelines bot commented Mar 18, 2024

MichalStrehovsky commented Mar 14, 2024 •

edited

Loading